feat/catalog transitions - #3194
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a680ef7a02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| params.migration?.draft === true || | ||
| matchingDraftPlanParams({ upsertProductPlan, params }) != null; |
There was a problem hiding this comment.
Honor per-plan opt-outs from catalog migration drafts
When a request sets catalog-level migration.draft: true but a plan explicitly sets migration: { draft: false }, this unconditional OR still claims that plan's rows. This violates the documented per-plan override behavior and creates migration drafts for customered edits that explicitly opted out; resolve the matching plan's boolean first and fall back to the request-level default only when it is unset.
Useful? React with 👍 / 👎.
| matchingDraftPlanParams({ upsertProductPlan, params })?.migration | ||
| ?.include_custom === true; | ||
| ?.include_custom === true || params.migration?.include_custom === true; |
There was a problem hiding this comment.
Honor per-plan include_custom overrides
When catalog-level include_custom is true and a claimed plan explicitly specifies include_custom: false, the OR still returns true, so that plan's draft includes customized customers despite its override. Use the matched plan's value when present and only fall back to the catalog-level value when it is undefined.
Useful? React with 👍 / 👎.
| }); | ||
| handleUpsertProductVersionSlugErrors({ updateCatalogPlan }); | ||
| handleUpsertProductActiveErrors({ params }); | ||
| handleActivePointerErrors({ updateCatalogPlan }); |
There was a problem hiding this comment.
Scope active-pointer validation to newly invalidated plans
When an organization already contains an archived active version beside a live sibling—a state the existing pin-archive flow deliberately persisted and version-identity-remove.test.ts asserts—projected.products carries that state into every subsequent catalog request. Calling this validator unconditionally therefore makes even unrelated feature or plan updates fail with 400 after deployment; compare against the original state or restrict the check to plans whose pointer/archive state this request changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
4 issues found across 7 files
Confidence score: 3/5
handleUpdateCatalogErrors.tsdoes not detect an orphaned active pointer when a live active version is removed as a single non-all_versionsversion with no customers, leaving this transition insufficiently handled; extend detection for this removal path and add coverage.matchingDraftPlanParams.tslets request-levelmigration.draftclaim every row, preventing per-plan flags from opting rows out despite the schema comment; reconcile the precedence logic with the intended per-plan override behavior.transition-impossibilities.test.tsdocuments F1 as accepted while asserting rejection and no archival, making the expected transition unclear; align the test header and assertion with the intended behavior.auto-draft.test.tscleans up using customer or plan IDs even though drafts persist with{scope}-update-{uid}IDs, so test data may remain after execution; delete using the generated migration draft ID.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/internal/catalogV2/actions/updateCatalog/errors/handleUpdateCatalogErrors.ts">
<violation number="1" location="server/src/internal/catalogV2/actions/updateCatalog/errors/handleUpdateCatalogErrors.ts:181">
P2: detectOrphanedActivePointers only catches the case where the active version remains in the projection as archived. When a live active version is removed as a single (non-all_versions) version with no customers and no rewards, stampRemoveWillArchive hard-deletes it (willArchive=false), so projectCatalog drops the row entirely. The plan's surviving versions then have no active row, activeRow is undefined, and the orphaned active pointer passes this check — exactly the state the feature docstring says must be prevented. Consider flagging plans that have a surviving non-archived version but no active row. Note the primary archive path (active version removed with a surviving version) is detected correctly.</violation>
</file>
<file name="server/tests/integration/catalog-v2/plans/validation/transition-impossibilities.test.ts">
<violation number="1" location="server/tests/integration/catalog-v2/plans/validation/transition-impossibilities.test.ts:26">
P3: The header documents F1 as "Red (current)" and says the update is "accepted," but the first test in this file asserts the opposite — it expects the orphaned-pointer archive to reject and the row to stay unarchived. The refusal logic landed in this PR ("refuse to leave a live plan on an archived version"), so the test is green now. Remove or rewrite the stale "Red (current)" paragraph so the contract comment matches the current behavior.</violation>
</file>
<file name="server/tests/integration/catalog-v2/plans/migrations/auto-draft.test.ts">
<violation number="1" location="server/tests/integration/catalog-v2/plans/migrations/auto-draft.test.ts:106">
P3: The migration draft persists with an id of `{scope}-update-{uid}` (see buildMigrationDraftId), never the plan id, but `finally` calls `deleteMigrations({ ctx, ids: [customeredId] })` / `[planId]`. Since every sibling draft test (billing-flag, customize-buckets, draft-guards, filter-collapse) deletes via `response.migrations![0]!.id`, passing the plan id here deletes nothing, and the draft row created by the request-level flag leaks in the test DB. Capture the migration id from the response (as the other tests do) and delete that.</violation>
</file>
<file name="server/src/internal/catalogV2/actions/updateCatalog/compute/computeMigrationDraftPlans/matchingDraftPlanParams.ts">
<violation number="1" location="server/src/internal/catalogV2/actions/updateCatalog/compute/computeMigrationDraftPlans/matchingDraftPlanParams.ts:87">
P2: A request-level `migration.draft` claims every row and short-circuits per-plan flags, so no plan entry can opt a row back out of drafting. The schema comment in this PR calls the field "overridable per plan," but `params.migration?.draft === true ||` runs before `matchingDraftPlanParams`, so `migration: { draft: false }` on a plan is ignored once the request-level flag is set. A config client that wants to exclude a specific plan from a request-wide draft has no way to do so.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }); | ||
| handleUpsertProductVersionSlugErrors({ updateCatalogPlan }); | ||
| handleUpsertProductActiveErrors({ params }); | ||
| handleActivePointerErrors({ updateCatalogPlan }); |
There was a problem hiding this comment.
P2: detectOrphanedActivePointers only catches the case where the active version remains in the projection as archived. When a live active version is removed as a single (non-all_versions) version with no customers and no rewards, stampRemoveWillArchive hard-deletes it (willArchive=false), so projectCatalog drops the row entirely. The plan's surviving versions then have no active row, activeRow is undefined, and the orphaned active pointer passes this check — exactly the state the feature docstring says must be prevented. Consider flagging plans that have a surviving non-archived version but no active row. Note the primary archive path (active version removed with a surviving version) is detected correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/catalogV2/actions/updateCatalog/errors/handleUpdateCatalogErrors.ts, line 181:
<comment>detectOrphanedActivePointers only catches the case where the active version remains in the projection as archived. When a live active version is removed as a single (non-all_versions) version with no customers and no rewards, stampRemoveWillArchive hard-deletes it (willArchive=false), so projectCatalog drops the row entirely. The plan's surviving versions then have no active row, activeRow is undefined, and the orphaned active pointer passes this check — exactly the state the feature docstring says must be prevented. Consider flagging plans that have a surviving non-archived version but no active row. Note the primary archive path (active version removed with a surviving version) is detected correctly.</comment>
<file context>
@@ -177,6 +178,7 @@ export const handleUpdateCatalogErrors = async ({
});
handleUpsertProductVersionSlugErrors({ updateCatalogPlan });
handleUpsertProductActiveErrors({ params });
+ handleActivePointerErrors({ updateCatalogPlan });
handleUpsertProductErrors({
updateCatalogPlan,
</file context>
| params: UpdateCatalogParams; | ||
| }): boolean => matchingDraftPlanParams({ upsertProductPlan, params }) != null; | ||
| }): boolean => | ||
| params.migration?.draft === true || |
There was a problem hiding this comment.
P2: A request-level migration.draft claims every row and short-circuits per-plan flags, so no plan entry can opt a row back out of drafting. The schema comment in this PR calls the field "overridable per plan," but params.migration?.draft === true || runs before matchingDraftPlanParams, so migration: { draft: false } on a plan is ignored once the request-level flag is set. A config client that wants to exclude a specific plan from a request-wide draft has no way to do so.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/catalogV2/actions/updateCatalog/compute/computeMigrationDraftPlans/matchingDraftPlanParams.ts, line 87:
<comment>A request-level `migration.draft` claims every row and short-circuits per-plan flags, so no plan entry can opt a row back out of drafting. The schema comment in this PR calls the field "overridable per plan," but `params.migration?.draft === true ||` runs before `matchingDraftPlanParams`, so `migration: { draft: false }` on a plan is ignored once the request-level flag is set. A config client that wants to exclude a specific plan from a request-wide draft has no way to do so.</comment>
<file context>
@@ -72,13 +72,20 @@ export const matchingDraftPlanParams = ({
params: UpdateCatalogParams;
-}): boolean => matchingDraftPlanParams({ upsertProductPlan, params }) != null;
+}): boolean =>
+ params.migration?.draft === true ||
+ matchingDraftPlanParams({ upsertProductPlan, params }) != null;
</file context>
| * F3 an internal_id rename carries the plan's whole history, pointer | ||
| * included — the row addressed is the handle, not the scope | ||
| * | ||
| * Red (current): nothing inspects the projected active pointer, so F1 is |
There was a problem hiding this comment.
P3: The header documents F1 as "Red (current)" and says the update is "accepted," but the first test in this file asserts the opposite — it expects the orphaned-pointer archive to reject and the row to stay unarchived. The refusal logic landed in this PR ("refuse to leave a live plan on an archived version"), so the test is green now. Remove or rewrite the stale "Red (current)" paragraph so the contract comment matches the current behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/catalog-v2/plans/validation/transition-impossibilities.test.ts, line 26:
<comment>The header documents F1 as "Red (current)" and says the update is "accepted," but the first test in this file asserts the opposite — it expects the orphaned-pointer archive to reject and the row to stay unarchived. The refusal logic landed in this PR ("refuse to leave a live plan on an archived version"), so the test is green now. Remove or rewrite the stale "Red (current)" paragraph so the contract comment matches the current behavior.</comment>
<file context>
@@ -0,0 +1,176 @@
+ * F3 an internal_id rename carries the plan's whole history, pointer
+ * included — the row addressed is the handle, not the scope
+ *
+ * Red (current): nothing inspects the projected active pointer, so F1 is
+ * accepted and the plan is left current-versionless.
+ * Green (after): F1 is a 400 naming the row in the way.
</file context>
| ], | ||
| }); | ||
| } finally { | ||
| await deleteMigrations({ ctx, ids: [customeredId] }); |
There was a problem hiding this comment.
P3: The migration draft persists with an id of {scope}-update-{uid} (see buildMigrationDraftId), never the plan id, but finally calls deleteMigrations({ ctx, ids: [customeredId] }) / [planId]. Since every sibling draft test (billing-flag, customize-buckets, draft-guards, filter-collapse) deletes via response.migrations![0]!.id, passing the plan id here deletes nothing, and the draft row created by the request-level flag leaks in the test DB. Capture the migration id from the response (as the other tests do) and delete that.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/catalog-v2/plans/migrations/auto-draft.test.ts, line 106:
<comment>The migration draft persists with an id of `{scope}-update-{uid}` (see buildMigrationDraftId), never the plan id, but `finally` calls `deleteMigrations({ ctx, ids: [customeredId] })` / `[planId]`. Since every sibling draft test (billing-flag, customize-buckets, draft-guards, filter-collapse) deletes via `response.migrations![0]!.id`, passing the plan id here deletes nothing, and the draft row created by the request-level flag leaks in the test DB. Capture the migration id from the response (as the other tests do) and delete that.</comment>
<file context>
@@ -0,0 +1,158 @@
+ ],
+ });
+ } finally {
+ await deleteMigrations({ ctx, ids: [customeredId] });
+ await cleanupPlanCustomerRefs({
+ ctx,
</file context>
Stack created with GitHub Stacks CLI • Give Feedback 💬
Summary by cubic
Adds catalog-wide migration drafts and a guard against leaving a plan without a valid active version. Previously a migration draft only appeared when the caller set
migration: { draft: true }on the exact entry; nowmigration.draft: trueat the request level claims every row and the server drafts only where customers and a diff warrant it. Archiving the active version of a plan while live sibling versions remain is now rejected with a 400; archiving the entire plan is still allowed.migration.draft: truecovers every row, so config-file clients pushing the whole catalog no longer need to know which plans hold customers.migrationparams still work and override the request-level default.Written for commit a680ef7. Summary will update on new commits.